Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enhance: Add compaction task slot usage logic #34581

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

wayblink
Copy link
Contributor

@sre-ci-robot sre-ci-robot added the size/L Denotes a PR that changes 100-499 lines. label Jul 10, 2024
@sre-ci-robot sre-ci-robot requested review from bigsheeper and sunby July 10, 2024 10:32
@mergify mergify bot added dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement labels Jul 10, 2024
Copy link
Contributor

mergify bot commented Jul 10, 2024

@wayblink E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@wayblink
Copy link
Contributor Author

/run-cpu-e2e

@wayblink wayblink force-pushed the clustering-control branch 2 times, most recently from 287a20b to 7abe847 Compare July 10, 2024 15:08
@wayblink
Copy link
Contributor Author

rerun ut

@mergify mergify bot added the ci-passed label Jul 11, 2024
Copy link

codecov bot commented Jul 11, 2024

Codecov Report

Attention: Patch coverage is 83.18584% with 19 lines in your changes missing coverage. Please review.

Project coverage is 80.77%. Comparing base (17c96e1) to head (eefcccb).
Report is 11 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #34581    +/-   ##
========================================
  Coverage   80.76%   80.77%            
========================================
  Files        1161     1161            
  Lines      141018   141148   +130     
========================================
+ Hits       113898   114017   +119     
- Misses      22755    22760     +5     
- Partials     4365     4371     +6     
Files Coverage Δ
internal/datacoord/compaction_task_l0.go 68.29% <100.00%> (+0.12%) ⬆️
internal/datacoord/compaction_task_mix.go 63.09% <100.00%> (+1.31%) ⬆️
...ternal/datanode/compaction/clustering_compactor.go 61.94% <100.00%> (+<0.01%) ⬆️
pkg/util/merr/errors.go 88.23% <ø> (ø)
pkg/util/paramtable/component_param.go 98.52% <100.00%> (+0.01%) ⬆️
internal/datacoord/compaction.go 71.15% <93.33%> (+0.55%) ⬆️
internal/datanode/compaction/executor.go 90.79% <94.11%> (+0.49%) ⬆️
internal/datanode/compaction/l0_compactor.go 90.94% <50.00%> (-0.63%) ⬇️
internal/datanode/compaction/mix_compactor.go 70.64% <50.00%> (-0.66%) ⬇️
internal/datanode/services.go 85.94% <60.00%> (-0.87%) ⬇️
... and 2 more

... and 32 files with indirect coverage changes

@wayblink wayblink force-pushed the clustering-control branch from 7abe847 to 918d293 Compare July 11, 2024 12:34
@wayblink wayblink changed the title enhance: Only allow one clustering compaction task executing on each dataNode enhance: Support set compaction task slot usage in scheduling Jul 11, 2024
@mergify mergify bot removed the ci-passed label Jul 11, 2024
@wayblink wayblink force-pushed the clustering-control branch from 918d293 to 9b0e8db Compare July 11, 2024 12:41
@wayblink wayblink force-pushed the clustering-control branch 2 times, most recently from 3e0ff3c to b4b9eaa Compare July 12, 2024 07:00
@mergify mergify bot added the ci-passed label Jul 12, 2024
@wayblink wayblink force-pushed the clustering-control branch from b4b9eaa to afc4110 Compare July 15, 2024 07:23
@mergify mergify bot removed the ci-passed label Jul 15, 2024
nodeID = id
maxSlots = slots
}
}
// update the input nodeSlots
nodeSlots[nodeID] = nodeSlots[nodeID] - acquireSlot
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should decrease the slots after nodeID meta saved

@@ -558,7 +566,7 @@ func (t *clusteringCompactionTask) GetLabel() string {
}

func (t *clusteringCompactionTask) NeedReAssignNodeID() bool {
return t.GetState() == datapb.CompactionTaskState_pipelining && t.GetNodeID() == 0
return t.GetState() == datapb.CompactionTaskState_pipelining && (t.GetNodeID() == 0 || t.GetNodeID() == NullNodeID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dup with pr: #34537

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I learn this from your pr

dropped *typeutil.ConcurrentSet[string] // vchannel dropped
usingSlots *atomic.Int64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it is a double check. Executor can refuse new task even datacoord works unexpectedly.

@@ -37,7 +39,7 @@ const (

type Executor interface {
Start(ctx context.Context)
Execute(task Compactor)
Execute(task Compactor) (bool, error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, if slots are used up. return a DataNodeSlotExhausted error

@wayblink wayblink force-pushed the clustering-control branch 4 times, most recently from 3f61843 to 27d07dd Compare July 16, 2024 06:25
@wayblink wayblink force-pushed the clustering-control branch 2 times, most recently from b7c26b4 to 28b8d7c Compare July 17, 2024 02:00
@wayblink wayblink changed the title enhance: Support set compaction task slot usage in scheduling enhance: Add compaction task slot usage logic Jul 17, 2024
@wayblink wayblink force-pushed the clustering-control branch 4 times, most recently from 377995e to a5a43c2 Compare July 17, 2024 06:31
@wayblink wayblink force-pushed the clustering-control branch from a5a43c2 to eefcccb Compare July 17, 2024 06:53
@mergify mergify bot added the ci-passed label Jul 17, 2024
sre-ci-robot pushed a commit that referenced this pull request Jul 18, 2024
@czs007
Copy link
Collaborator

czs007 commented Jul 18, 2024

/approve
/lgtm

@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: czs007, wayblink

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sre-ci-robot sre-ci-robot merged commit c79d1af into milvus-io:master Jul 18, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved ci-passed dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement lgtm size/L Denotes a PR that changes 100-499 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants